feat: emit OVOS-INTENT-4 registration topics (dual-emit with legacy)#431
feat: emit OVOS-INTENT-4 registration topics (dual-emit with legacy)#431JarbasAl wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds spec-topic emission for intent registration, entity/template registration, context syncing, and deregistration; updates ChangesINTENT-4 Producer Implementation and Tests
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greetings, human! The automated checks are complete. 👾I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's hydration (aka documentation density). 💧 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckEnsuring no copyleft violations in this PR. ⬅️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Checking if we're following security best practices. 📏 ✅ No known vulnerabilities found (73 packages scanned). 🔨 Build TestsChecking the blueprint against the actual construction. 📐
❌ 3.10: Install OK, tests failed 🔍 LintAnother piece of the puzzle! 🧩 ❌ ruff: issues found — see job log Catch you at the next merge! 🌊 |
JarbasAl
force-pushed
the
feat/intent-4-producer
branch
from
6fa23bc to
977cfa2
Compare
JarbasAl
force-pushed
the
feat/intent-4-producer
branch
from
977cfa2 to
79338f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pyproject.toml (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCap the
ovos-coretest dependency.
pip install .[test]can now resolve a future breakingovos-corerelease even though the rest of this extras set is version-capped. That makes local test environments drift from the stack this PR is actually validating. Please add the intended upper bound here as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 29, The test extras currently leave ovos-core uncapped, which can let pip install .[test] resolve a newer breaking release than the rest of the stack. Update the test dependency entry in pyproject.toml to add the same intended upper bound as the other related extras, keeping the version constraints aligned with the validation target.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ovoscope.yml:
- Around line 35-43: The e2e install step is using floating `@dev` refs in the
workflow, which makes the test stack unstable. Update the pip install entries in
the workflow to use pinned commit SHAs or a constraints file instead of
branch-head URLs, and keep the pinned versions intentional and consistent across
the stack. Reference the install block in the workflow that includes ovos-core,
ovos-adapt-pipeline-plugin, ovos-padatious-pipeline-plugin, ovoscope, and
padacioso.
- Around line 17-21: The workflow currently uses mutable action tags and leaves
checkout credentials persisted, which exposes GITHUB_TOKEN to later external
repo installs. Update the actions used in the ovoscope job by pinning both
actions to immutable commit SHAs, and configure the checkout step in the
existing actions/checkout usage to set persist-credentials to false. Keep the
change in the same checkout and setup-python steps so the job still behaves the
same while reducing token exposure.
In `@ovos_workshop/intents.py`:
- Around line 263-276: The intent spec builder is emitting partial language
registrations because it computes langs from any cached referenced vocab while
_spec_keyword_descriptors silently skips missing keyword vocabs. Update the
intent registration/spec path that builds langs and calls
_spec_keyword_descriptors so a language is only included when all required
keyword vocab types for that intent are cached, or otherwise skip that language
entirely. Use _spec_keyword_descriptors and the surrounding spec payload
construction to enforce all-or-nothing keyword registration per lang.
- Around line 303-304: Rename the ambiguous `l` variable used in
`Intent`-related sample collection to a clearer name in the set comprehension
inside `intents.py` so Ruff no longer flags it; update the comprehension in the
logic that builds `langs` from `self._adapt_keyword_samples` and keep the
`referenced` filtering unchanged.
- Around line 389-391: The deregistration flow is emitting the stored munged
intent name instead of the canonical one. Update the `IntentContainer`
deregister path that calls
`self.bus.emit(msg.forward(SpecMessage.INTENT_DEREGISTER, ...))` so it
normalizes names the same way as registration, using the existing unmunge logic
before sending `intent_name`. Make sure `detach_all()` and `remove_intent()`
both end up emitting the canonical Padatious intent name rather than a stored
`.intent` suffix form.
In `@test/unittests/test_intent4_producer.py`:
- Around line 149-160: The test setup in Intent4 producer is using fixed /tmp
paths for intent and entity fixtures, which can collide across runs or leave
stale files behind. Update the setup/teardown in test_intent4_producer.py around
the intent_file/entity_file handling to use unique temporary files or a
TemporaryDirectory per test case, and keep the existing cleanup logic scoped to
those generated paths so each test instance remains isolated.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 29: The test extras currently leave ovos-core uncapped, which can let pip
install .[test] resolve a newer breaking release than the rest of the stack.
Update the test dependency entry in pyproject.toml to add the same intended
upper bound as the other related extras, keeping the version constraints aligned
with the validation target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b04ef7a-f1ff-4ad1-a943-0183430d774c
📒 Files selected for processing (17)
.github/workflows/ovoscope.ymlovos_workshop/intents.pypyproject.tomltest/end2end/__init__.pytest/end2end/intent4_test_skill/__init__.pytest/end2end/intent4_test_skill/locale/en-us/engine.entitytest/end2end/intent4_test_skill/locale/en-us/play.intenttest/end2end/test_intent4_producer_e2e.pytest/unittests/skills/temp_config/mycroft/apps/ovos.common_play/settings.jsontest/unittests/skills/temp_config/mycroft/apps/persona.openvoiceos/settings.jsontest/unittests/skills/temp_config/mycroft/apps/stop.openvoiceos/settings.jsontest/unittests/skills/temp_config/mycroft/skills/demo-layers.test/settings.jsontest/unittests/skills/temp_config/mycroft/skills/test_base_skill/settings.jsontest/unittests/skills/temp_config/mycroft/skills/test_fallback_v2/settings.jsontest/unittests/skills/temp_config/mycroft/skills/test_ovos_skill/settings.jsontest/unittests/test_intent4_producer.pytest/unittests/test_intent_service_interface.py
| - uses: actions/checkout@v6 | ||
| - name: Setup Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/ovoscope.yml"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant lines =="
nl -ba "$file" | sed -n '1,120p'
echo
echo "== search for external repo installs =="
rg -n "github.com|pip install|git clone|uv pip|poetry install|npm install|cargo install" "$file" || trueRepository: OpenVoiceOS/ovos-workshop
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/ovoscope.yml"
echo "== file with line numbers =="
awk '{printf "%4d %s\n", NR, $0}' "$file" | sed -n '1,220p'
echo
echo "== external fetch/install patterns =="
rg -n "github.com|git clone|pip install|poetry install|uv pip|npm install|cargo install|wget|curl" "$file" || trueRepository: OpenVoiceOS/ovos-workshop
Length of output: 2835
Pin the workflow actions and disable persisted checkout credentials.
actions/checkout leaves GITHUB_TOKEN in local git config by default, and this job later installs packages from several external GitHub repos. That gives any code executed here an unnecessary path to the repo token. Pin both actions to immutable SHAs and set persist-credentials: false on checkout.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 17-17: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 17-17: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ovoscope.yml around lines 17 - 21, The workflow currently
uses mutable action tags and leaves checkout credentials persisted, which
exposes GITHUB_TOKEN to later external repo installs. Update the actions used in
the ovoscope job by pinning both actions to immutable commit SHAs, and configure
the checkout step in the existing actions/checkout usage to set
persist-credentials to false. Keep the change in the same checkout and
setup-python steps so the job still behaves the same while reducing token
exposure.
Source: Linters/SAST tools
| run: | | ||
| pip install \ | ||
| "git+https://github.com/OpenVoiceOS/ovos-core@dev" \ | ||
| "git+https://github.com/OpenVoiceOS/ovos-adapt-pipeline-plugin@dev" \ | ||
| "git+https://github.com/OpenVoiceOS/ovos-padatious-pipeline-plugin@dev" \ | ||
| "git+https://github.com/OpenVoiceOS/ovoscope@dev" \ | ||
| "git+https://github.com/OpenVoiceOS/padacioso@dev" \ | ||
| "ovos-bus-client>=2.4.0a1" \ | ||
| "ovos-spec-tools>=0.16.1a2" \ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow and nearby context.
git ls-files .github/workflows/ovoscope.yml
wc -l .github/workflows/ovoscope.yml
sed -n '1,120p' .github/workflows/ovoscope.ymlRepository: OpenVoiceOS/ovos-workshop
Length of output: 2058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any local pinning/constraints that would make the stack deterministic.
rg -n --hidden --glob '!**/.git/**' \
'ovos-test-harness|constraints|requirements|`@dev`|git\+https://github.com/OpenVoiceOS/' .Repository: OpenVoiceOS/ovos-workshop
Length of output: 4143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find repository docs or workflow comments that describe the intended pinning model.
rg -n --hidden --glob '!**/.git/**' \
'Run Ovoscope End-to-End Tests|real e2e stack|working pin-set|spec-stack combo selector|ovoscope' .Repository: OpenVoiceOS/ovos-workshop
Length of output: 2311
Avoid floating @dev refs in the e2e stack. These installs track moving upstream branch heads, so CI can flip red without any change here. Pin the stack to commit SHAs or a constraints file and update it intentionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ovoscope.yml around lines 35 - 43, The e2e install step is
using floating `@dev` refs in the workflow, which makes the test stack unstable.
Update the pip install entries in the workflow to use pinned commit SHAs or a
constraints file instead of branch-head URLs, and keep the pinned versions
intentional and consistent across the stack. Reference the install block in the
workflow that includes ovos-core, ovos-adapt-pipeline-plugin,
ovos-padatious-pipeline-plugin, ovoscope, and padacioso.
| def _spec_keyword_descriptors(self, vocab_types: List[str], lang: str | ||
| ) -> List[dict]: | ||
| """Build OVOS-INTENT-4 §5.1 ``{name, samples}`` descriptors for the | ||
| given vocab names in ``lang``, inlining the cached samples. Vocabs with | ||
| no cached samples for this lang are skipped (no descriptor emitted). | ||
| """ | ||
| descriptors = [] | ||
| for vocab_type in vocab_types: | ||
| samples = self._adapt_keyword_samples.get((vocab_type, lang)) | ||
| if not samples: | ||
| continue | ||
| descriptors.append({"name": self._unmunge_vocab_name(vocab_type), | ||
| "samples": list(samples)}) | ||
| return descriptors |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Skip incomplete keyword registrations per language.
langs is built from any cached referenced vocab, while _spec_keyword_descriptors drops missing vocabs. If an intent requires setKW and brightnessKW but only setKW is cached for a lang, the spec payload emits a weaker required list than the legacy intent.
Proposed fix
for lang in langs:
+ missing = sorted(vocab_type for vocab_type in referenced
+ if not self._adapt_keyword_samples.get((vocab_type, lang)))
+ if missing:
+ LOG.debug(f"incomplete cached adapt vocab samples for intent "
+ f"{name} ({lang}); missing {missing}; skipping "
+ f"{SpecMessage.INTENT_REGISTER_KEYWORD} emit")
+ continue
payload = {
"skill_id": self.skill_id,
"intent_name": intent_name,Also applies to: 316-328
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ovos_workshop/intents.py` around lines 263 - 276, The intent spec builder is
emitting partial language registrations because it computes langs from any
cached referenced vocab while _spec_keyword_descriptors silently skips missing
keyword vocabs. Update the intent registration/spec path that builds langs and
calls _spec_keyword_descriptors so a language is only included when all required
keyword vocab types for that intent are cached, or otherwise skip that language
entirely. Use _spec_keyword_descriptors and the surrounding spec payload
construction to enforce all-or-nothing keyword registration per lang.
| langs = {l for (vt, l) in self._adapt_keyword_samples | ||
| if vt in referenced} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the ambiguous l variable.
Ruff flags l as ambiguous here.
Proposed fix
- langs = {l for (vt, l) in self._adapt_keyword_samples
+ langs = {lang for (vt, lang) in self._adapt_keyword_samples
if vt in referenced}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| langs = {l for (vt, l) in self._adapt_keyword_samples | |
| if vt in referenced} | |
| langs = {lang for (vt, lang) in self._adapt_keyword_samples | |
| if vt in referenced} |
🧰 Tools
🪛 Ruff (0.15.18)
[error] 303-303: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ovos_workshop/intents.py` around lines 303 - 304, Rename the ambiguous `l`
variable used in `Intent`-related sample collection to a clearer name in the set
comprehension inside `intents.py` so Ruff no longer flags it; update the
comprehension in the logic that builds `langs` from
`self._adapt_keyword_samples` and keep the `referenced` filtering unchanged.
Source: Linters/SAST tools
| self.bus.emit(msg.forward(SpecMessage.INTENT_DEREGISTER, | ||
| {"skill_id": self.skill_id, | ||
| "intent_name": intent_name})) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unmunge deregister intent names before emitting the spec topic.
Padatious registration emits play on the spec topic, but detach_all()/remove_intent() can pass the stored play.intent name. Emit the same canonical name for deregistration.
Proposed fix
self.bus.emit(msg.forward(SpecMessage.INTENT_DEREGISTER,
{"skill_id": self.skill_id,
- "intent_name": intent_name}))
+ "intent_name": unmunge_intent_name(intent_name)}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.bus.emit(msg.forward(SpecMessage.INTENT_DEREGISTER, | |
| {"skill_id": self.skill_id, | |
| "intent_name": intent_name})) | |
| self.bus.emit(msg.forward(SpecMessage.INTENT_DEREGISTER, | |
| {"skill_id": self.skill_id, | |
| "intent_name": unmunge_intent_name(intent_name)})) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ovos_workshop/intents.py` around lines 389 - 391, The deregistration flow is
emitting the stored munged intent name instead of the canonical one. Update the
`IntentContainer` deregister path that calls
`self.bus.emit(msg.forward(SpecMessage.INTENT_DEREGISTER, ...))` so it
normalizes names the same way as registration, using the existing unmunge logic
before sending `intent_name`. Make sure `detach_all()` and `remove_intent()`
both end up emitting the canonical Padatious intent name rather than a stored
`.intent` suffix form.
39219db to
61adca6
Compare
JarbasAl
force-pushed
the
feat/intent-4-producer
branch
3 times, most recently
from
ec7e17e to
a195c65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unittests/skills/test_intent_layers_e2e.py (1)
180-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider linking a tracking issue for the xfail'd bug.
Reasonable to defer the pre-existing layer-context-token bug out of this PR's scope, but with no linked issue,
strict=Falsexfail can go stale silently while masking the gating defect indefinitely.Also applies to: 241-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unittests/skills/test_intent_layers_e2e.py` around lines 180 - 184, The xfail on the intent layer e2e test currently documents the pre-existing layer-context-token bug but does not point to a tracking issue, so update the pytest.mark.xfail on the affected tests in test_intent_layers_e2e.py to reference the issue ID or link for the gating defect. Keep the existing reason text, but make sure the xfail annotation for the named test cases clearly ties the deferred failure to a tracked bug so it does not become an unmonitored permanent suppression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unittests/test_intent4_producer.py`:
- Around line 258-269: The test fixture for OVOSSkill.register_entity_file is
hardcoding the internal entity name with "engine" and md5("engine"), which no
longer matches the production munging logic. Update
test_entity_name_strips_hash_munge to derive the expected internal_name from the
normalized entity_file path used by register_entity_file (basename without
.entity plus md5 of that full path), so the assertion mirrors the behavior in
OVOSSkill.register_entity_file and register_padatious_entity.
---
Nitpick comments:
In `@test/unittests/skills/test_intent_layers_e2e.py`:
- Around line 180-184: The xfail on the intent layer e2e test currently
documents the pre-existing layer-context-token bug but does not point to a
tracking issue, so update the pytest.mark.xfail on the affected tests in
test_intent_layers_e2e.py to reference the issue ID or link for the gating
defect. Keep the existing reason text, but make sure the xfail annotation for
the named test cases clearly ties the deferred failure to a tracked bug so it
does not become an unmonitored permanent suppression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8213e3a6-068e-4167-9ca9-5667124a56df
📒 Files selected for processing (12)
.gitignoreovos_workshop/intents.pyovos_workshop/skills/ovos.pytest/end2end/__init__.pytest/end2end/intent4_test_skill/__init__.pytest/end2end/intent4_test_skill/locale/en-us/engine.entitytest/end2end/intent4_test_skill/locale/en-us/play.intenttest/end2end/test_intent4_producer_e2e.pytest/unittests/skills/test_base.pytest/unittests/skills/test_intent_layers_e2e.pytest/unittests/test_intent4_producer.pytest/unittests/test_intent_service_interface.py
✅ Files skipped from review due to trivial changes (3)
- test/end2end/intent4_test_skill/locale/en-us/play.intent
- .gitignore
- test/end2end/intent4_test_skill/locale/en-us/engine.entity
🚧 Files skipped from review as they are similar to previous changes (2)
- test/end2end/intent4_test_skill/init.py
- test/end2end/test_intent4_producer_e2e.py
| def test_entity_name_strips_hash_munge(self): | ||
| entity_file = "/tmp/intent4_real_engine.entity" | ||
| with open(entity_file, "w") as f: | ||
| f.write("spotify\nyoutube music\n") | ||
| try: | ||
| # OVOSSkill.register_entity_file builds | ||
| # `<skill_id>:<basename>_<md5(entity_file)>` | ||
| basename = "engine" | ||
| digest = md5("engine".encode("utf-8")).hexdigest() | ||
| internal_name = f"{self.SKILL_ID}:{basename}_{digest}" | ||
| self.iface.register_padatious_entity(internal_name, entity_file, | ||
| lang="en-US") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how OVOSSkill.register_entity_file actually constructs the munged entity name
rg -n -B3 -A10 'register_entity_file' ovos_workshop/skills/ovos.pyRepository: OpenVoiceOS/ovos-workshop
Length of output: 1291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the production implementation and any helpers that build the internal entity name
sed -n '1359,1425p' ovos_workshop/skills/ovos.py
printf '\n---\n'
rg -n -B3 -A8 'md5|hash|basename|entity_file|register_padatious_entity|register_entity_file' ovos_workshop/skills/ovos.py test/unittests/test_intent4_producer.pyRepository: OpenVoiceOS/ovos-workshop
Length of output: 15360
Build the entity name from the normalized file path. OVOSSkill.register_entity_file() strips .entity, then uses basename(entity_file) plus md5(entity_file.encode("utf-8")); this fixture hardcodes engine/md5("engine"), so it no longer mirrors the production munging logic.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 258-258: Do not hardcode temporary file or directory names
Context: "/tmp/intent4_real_engine.entity"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[warning] 259-259: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(entity_file, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 265-265: Do not use insecure functions
Context: md5("engine".encode("utf-8"))
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(insecure-hash-functions)
🪛 Ruff (0.15.20)
[error] 259-259: Probable insecure usage of temporary file or directory: "/tmp/intent4_real_engine.entity"
(S108)
[error] 266-266: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unittests/test_intent4_producer.py` around lines 258 - 269, The test
fixture for OVOSSkill.register_entity_file is hardcoding the internal entity
name with "engine" and md5("engine"), which no longer matches the production
munging logic. Update test_entity_name_strips_hash_munge to derive the expected
internal_name from the normalized entity_file path used by register_entity_file
(basename without .entity plus md5 of that full path), so the assertion mirrors
the behavior in OVOSSkill.register_entity_file and register_padatious_entity.
…431) (#452) * refactor: drop generic register_regex; deprecate the back-compat facade Regex intents are adapt-engine only with no spec equivalent, so there is no generic `register_regex` on IntentServiceInterface — the sole entry point is `register_adapt_regex` (self-contained; munges + emits the legacy adapt vocab). Skill regex loaders call `register_adapt_regex`. Every IntentServiceInterface back-compat facade delegate now emits a DeprecationWarning pointing at the spec-compliant replacement (keyword/template registration, set_intent_context, spec deregistration). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat: OVOS-CONTEXT-1 §6 producer — emit requires_context/excludes_context Attach the CONTEXT-1 gating declarations to the INTENT-4 registration payloads so the engine-side gate is fed by real skills: - ovos.intent.register.keyword and ovos.intent.register.template payloads carry requires_context / excludes_context (default [], shape-stable). - register_intent / register_intent_file (OVOSSkill) and the @intent_handler decorator gain optional requires_context / excludes_context params, threaded through to the emit. - OVOSSkill.set_intent_context / remove_intent_context expose the §5.3 surface. - IntentLayers activate a real requires_context: ["layer_<name>"] gate (engine-agnostic) via set_intent_context; the legacy adapt layer_<name> vocab token is kept as a deprecated bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: pass requires/excludes_context to register_template only when set Keeps the register_template call shape unchanged for the common (no-context) path so existing callers and their mocks are unaffected; the payload stays shape-stable because register_template defaults the fields to []. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovos_workshop/skills/ovos.py (1)
2382-2390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass bare intent names to
remove_intent
remove_intentmatches against bare entries inintent_names, so the skill-id-prefixed value here never hits the detach bookkeeping and emitsINTENT_DEREGISTERfor the wrong name.detach()has the same issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ovos_workshop/skills/ovos.py` around lines 2382 - 2390, The intent removal paths are passing skill-id-prefixed names into remove_intent, but it matches against bare entries in intent_names, so the deregistration bookkeeping misses the intended intent and emits the wrong name. Update the disable-intent logic in ovos.py to pass the bare intent_name to remove_intent, and make the same adjustment in detach() wherever it removes intents. Keep the language-suffixed removals aligned with the same bare base name so the intent service can match and deregister correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 2382-2390: The intent removal paths are passing skill-id-prefixed
names into remove_intent, but it matches against bare entries in intent_names,
so the deregistration bookkeeping misses the intended intent and emits the wrong
name. Update the disable-intent logic in ovos.py to pass the bare intent_name to
remove_intent, and make the same adjustment in detach() wherever it removes
intents. Keep the language-suffixed removals aligned with the same bare base
name so the intent service can match and deregister correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 791d9046-2913-4347-809c-938824278955
📒 Files selected for processing (6)
ovos_workshop/decorators/__init__.pyovos_workshop/decorators/layers.pyovos_workshop/intents.pyovos_workshop/skills/ovos.pytest/unittests/test_decorators.pytest/unittests/test_intent4_producer.py
Restructure IntentServiceInterface into a spec-compliant OVOS-INTENT-4 / OVOS-CONTEXT-1 producer that dual-emits the SpecMessage registration topics (ovos.intent.register.keyword/template, ovos.entity.register, ovos.intent.deregister) alongside the legacy adapt/padatious bus topics. Deprecated adapt/padatious engine protocols are composed onto _AdaptIntentApi /_PadatiousIntentApi shims instead of inherited, so they can be deleted with their engines. Preserves dev's OVOS-INTENT-2 §4.3 blacklist work through the new producer path: intent slot_blacklist and entity/template blacklists are threaded through register_template/register_entity and their legacy dual-emit so the padatious:register_intent/entity payloads keep slot_blacklist/blacklist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JarbasAl
force-pushed
the
feat/intent-4-producer
branch
from
9360357 to
41c8649
Compare
1 participant
Adopts the OVOS-INTENT-4 producer contract in
IntentServiceInterface(ovos_workshop/intents.py) so skills EMIT the consolidated spec registration bus topics. This unblocks the six already-merged-ready engine consumer PRs (adapt / padatious / padacioso / nebulento / palavreado / m2v) that subscribe to these topics but currently receive nothing.Dual-emit vs bridge
Registration topics (
ovos.intent.register.keyword/.template,ovos.entity.register) are an N→1 consolidation and are deliberately NOT inovos_spec_tools.MIGRATION_MAP— the bus can't transparently bridge them. So we dual-emit: keep the legacy emits AND hand-emit the spec topic with the spec payload.Management topics (
detach_intent→ovos.intent.deregister) ARE inMIGRATION_MAP(1:1, payload restructured but bridged). Soremove_intentemits the spec topic only and lets the bus bridge to legacy — no hand-rolled legacy dual-emit.Payload mapping
register_adapt_intent→ovos.intent.register.keyword{skill_id, intent_name, lang, required, optional, one_of, excluded}. Each descriptor is{name, samples}. Samples are reconstructed from a per-(vocab_type, lang)cache populated byregister_adapt_keyword(entity + aliases, order-preserving, de-duped).required/optionalcome from parserrequires/optional(tuple[0]),one_offromat_least_one(array of groups, empty groups dropped),excludedfrom the flatexcludeslist. Vocabnameis un-munged on the wire (the consumer re-namespaces). One message per language the intent's vocab was cached under. All four keys always present (§5.2 shape-stable).register_padatious_intent→ovos.intent.register.template{skill_id, intent_name (skill-local), lang, samples, blacklist}(blacklistdefaults[]).register_padatious_entity→ovos.entity.register{skill_id, entity_name (skill-local), lang, samples}.remove_intent→ovos.intent.deregister{skill_id, intent_name (skill-local)}, spec-only (bus bridges to legacydetach_intent).Files changed
ovos_workshop/intents.py— sample cache + spec emits.pyproject.toml—ovos-spec-tools>=0.16.1a2(INTENT-4SpecMessagemembers).test/unittests/test_intent4_producer.py— new; asserts §5/§6/§7/§8 spec payloads via FakeBus.test/unittests/test_intent_service_interface.py— filter the legacy-topic assertions to tolerate the new dual-emit.Tests
Full suite green: 494 passed (
python -m pytest test/in~/.venvs/ovos).Known limitation
Inlined keyword samples are reconstructed from vocab registered via
register_adapt_keywordbefore the intent. An adapt intent that references a vocab with no cached samples for any language emits no keyword message (logged at DEBUG) — vocab must be registered before the intent that uses it (the normal skill load order).🤖 Generated with Claude Code
Summary by CodeRabbit